fix(webview): isolate parallel provider view state#909
fix(webview): isolate parallel provider view state#909easonLiangWorldedtech wants to merge 11 commits into
Conversation
Add view-local state loading and syncing for parallel ClineProvider instances, with safe initialization and configuration listener handling for test/shim environments. Key changes: - Add viewId property for unique provider instance identification - Implement viewLocalState buffer to isolate mode, profile, and apiConfig per view - Load view-local state after provider dependencies are initialized - Merge viewLocalState in getState() with local apiConfiguration taking precedence - Clear local overrides when saveViewState receives undefined or null values - Sync mode/profile changes through saveViewState(), loadViewState(), and profile activation - Preserve restored task mode in both global state and viewLocalState - Guard global configuration listener setup when VS Code workspace events are unavailable - Keep sticky-mode task restore compatible with view-local state isolation Tests: - Add 22 parallel mode cases covering viewId uniqueness, state isolation, save/load behavior, merge precedence, local override clearing, config listener handling, mode switching, profile activation, and multi-instance isolation - Verify sticky-mode restore compatibility
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughClineProvider now isolates mode and API profile state per view instance, synchronizes local state with shared context changes, and merges local overrides into ChangesParallel view state isolation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Webview
participant webviewMessageHandler
participant ClineProvider
participant ContextProxy
Webview->>webviewMessageHandler: webviewDidLaunch(viewStateId)
webviewMessageHandler->>ClineProvider: setViewStateId(viewStateId)
ClineProvider->>ContextProxy: loadViewState()
Webview->>ClineProvider: switch mode or activate provider profile
ClineProvider->>ClineProvider: saveViewState()
ClineProvider->>ContextProxy: update shared state
ClineProvider-->>Webview: post merged ExtensionState
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/webview/__tests__/ClineProvider.parallelMode.spec.ts`:
- Around line 1118-1143: Update the mode-switch section of the test “should
handle mode switch in one instance without affecting others” to call
provider1.handleModeSwitch("architect") instead of the private saveViewState
method, while preserving the existing state assertions for both providers.
- Around line 829-857: Update the configuration-change test around ClineProvider
and setupGlobalStateListener to capture the registered onDidChangeConfiguration
handler, spy on loadViewState(), invoke the handler with configChangeEvent, and
assert that loadViewState() is called. Remove the unused event-only setup and
the weak disposables.length assertion.
In `@src/core/webview/ClineProvider.ts`:
- Around line 449-458: Update saveViewState() to persist each value through a
view-specific ContextProxy key derived from the current viewId, rather than the
shared ExtensionState key. Keep the existing viewLocalState cache update
behavior, and ensure corresponding view-state reads use the same derived key so
recreated views restore their own mode, profile, and configuration.
- Around line 1600-1602: Update the mode-switch flow around
saveViewState("mode", newMode) so the no-saved-configuration path reads the
profile configuration name from this view’s getState() result rather than shared
currentApiConfigName. Preserve the existing mode-switch behavior while ensuring
another tab’s global state cannot supply this view’s configuration.
- Around line 2674-2676: Update the state mutation paths around setMode(),
setValues(), profile upsert/deletion, and resetState() so local-key changes also
update or invalidate viewLocalState instead of being hidden by
mergedStateValues. Centralize the local-key mutation handling, and explicitly
clear viewLocalState during resetState() so reset returns current persisted
defaults rather than stale mode, profile, or configuration values.
- Around line 241-243: Update the viewId initialization in the ClineProvider
constructor to use a monotonically increasing instance identifier rather than
ClineProvider.activeInstances.size, ensuring IDs remain unique after instances
are disposed; keep activeInstances tracking unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4b3c9918-8ed9-4d37-8d1e-80201da59ec5
📒 Files selected for processing (2)
src/core/webview/ClineProvider.tssrc/core/webview/__tests__/ClineProvider.parallelMode.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/webview/__tests__/ClineProvider.parallelMode.spec.ts`:
- Around line 750-752: Update the view-state key construction used by
saveViewState and the related test expectation so it uses a stable per-view
identifier rather than the construction-order suffix from viewId. Ensure
restored tabs consistently read and write their own persisted mode and API
configuration, and remove reliance on the shared-key fallback for this state.
- Around line 862-877: Update loadViewState in ClineProvider to read
apiConfiguration from the view-specific persisted state key rather than shared
provider settings, preserving the tab’s model/provider configuration across
reloads. Add or adjust coverage in the parallel-mode tests by seeding per-view
apiConfiguration and asserting it is restored after loading, while keeping
existing failure behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 530ae001-d224-4694-b647-70aef27ee0f4
📒 Files selected for processing (5)
src/core/webview/ClineProvider.tssrc/core/webview/__tests__/ClineProvider.parallelMode.spec.tssrc/core/webview/__tests__/ClineProvider.spec.tssrc/extension/__tests__/api-set-configuration.spec.tssrc/extension/api.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/webview/ClineProvider.ts
github-actions
Bot
added
the
awaiting-review
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/webview/webviewMessageHandler.ts`:
- Around line 561-562: Update downstream state access in the webview message
handler to use view-local state after setViewStateId initializes the provider.
Replace the currentApiConfigName read near line 607 with the current value from
await provider.getState(), and replace both updateGlobalState("mode", ...) calls
near lines 2200 and 2296 with await provider.handleModeSwitch(...) so mode
changes apply to the active tab.
In `@webview-ui/src/App.tsx`:
- Line 194: Remove the duplicate webviewDidLaunch useEffect from App, keeping
the launch handshake in ExtensionStateContextProvider; update
webview-ui/src/App.tsx lines 194-194 and leave
webview-ui/src/context/ExtensionStateContext.tsx lines 492-492 unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 045a32de-dd60-4dec-9b66-4f1e3da5467f
📒 Files selected for processing (8)
packages/types/src/vscode-extension-host.tssrc/core/webview/ClineProvider.tssrc/core/webview/__tests__/ClineProvider.parallelMode.spec.tssrc/core/webview/webviewMessageHandler.tswebview-ui/src/App.tsxwebview-ui/src/__tests__/App.spec.tsxwebview-ui/src/context/ExtensionStateContext.tsxwebview-ui/src/utils/vscode.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/webview/ClineProvider.ts
- src/core/webview/tests/ClineProvider.parallelMode.spec.ts
Read launch profile state from the active provider view and route mode changes through handleModeSwitch. Remove the duplicate webviewDidLaunch handshake from App so initialization only runs once via ExtensionStateContextProvider.
github-actions
Bot
added
awaiting-review
taltas
left a comment
taltas
left a comment
There was a problem hiding this comment.
Blocking findings:
-
Cross-view configuration merging can inject another profile settings, including credentials.
src/core/webview/ClineProvider.ts:2803-2807starts with the sharedContextProxyprovider settings and overlays the local profile as a sparse object. If view B last activated an OpenRouter profile with an API key and view A local OpenRouter profile omits that key, A keeps B key from the first spread;ContextProxy.setProviderSettings()also clears only absent non-secret fields (src/core/config/ContextProxy.ts:510-531). A subsequent task consumes this contaminated object atClineProvider.ts:1295-1301. Resolve the selected local profile as an authoritative complete configuration and explicitly clear absent fields; add a two-view test where one same-provider profile intentionally omits a secret. -
The stable persistence path neither persists profile/configuration mutations nor survives a real
ContextProxyreconstruction.loadViewState()reads dynamic keys atClineProvider.ts:456-476, but repository search finds only one productionsaveViewState()call, for mode atClineProvider.ts:1644-1647; activation/upsert only mutate memory atClineProvider.ts:1790-1792and1886-1888. Even the mode key is unavailable after extension-host restart becauseContextProxy.initialize()loads only declaredGLOBAL_STATE_KEYS(src/core/config/ContextProxy.ts:59-67), whilegetValue()reads that cache. Thus a reload with the same frontend ID falls back to whichever shared profile another view last wrote. The test mock atClineProvider.parallelMode.spec.ts:271-285reads arbitrary Memento keys directly and masks this. Store a declared, bounded per-view map or use a dedicated dynamic-key storage API, persist every canonical mutation path, and persist profile identity rather than copying secret-bearing API configuration into global state. -
Mode-specific profile selection is still globally shared. Profile activation writes
ProviderSettingsManager.setModeConfig(mode, id)atClineProvider.ts:1890-1894, and every view reads that same mapping when switching modes atClineProvider.ts:1658-1680. If two views incodeselect P1 and P2, P2 overwrites the shared mapping; when the first view switches away and back, it activates P2. Scope the mode-to-profile mapping by stable view identity or keep it entirely in the per-view state transition. -
Globally shared mode data is frozen into each local buffer.
loadViewState()snapshotscustomModePromptsandmodeApiConfigsatClineProvider.ts:470-475, and the broad overlay atClineProvider.ts:2725-2729,2857-2860keeps those snapshots ahead of later shared writes. For example, prompt edits update global state atsrc/core/webview/webviewMessageHandler.ts:1747-1758, but another view can repost its old prompt. The listener atClineProvider.ts:535-558observes VS Code workspace configuration, notExtensionContext.globalStateor SecretStorage writes, so it cannot reconcile this. Restrict the local buffer to truly isolated fields and broadcast explicit shared-state invalidations. -
Global reset/import operations leave other live views on stale or erased state.
resetState()clears shared state and profile secrets but only clears the invoking provider buffer/task (ClineProvider.ts:3117-3123), so another open view can retain its old profile, API handler, credentials, and task after reset. Settings import writes directly throughContextProxy, with active-profile writes not awaited (src/core/config/importExport.ts:229-244), then reposts only the invoking provider, whose old local overrides can hide the import (importExport.ts:385-388). Make these operations coordinate all active providers, await all storage writes, invalidate every local snapshot, reconcile active tasks, and clear persisted per-view records.
The focused changed suites pass (81 extension-host tests and 18 webview tests), both extension and webview typechecks pass, and the full webview suite passes 1,458 tests. The complete extension suite reached 6,816 passing tests but exited nonzero on six worker teardown errors attributed to the new parallel-mode suite. The new coverage does not reconstruct a real ContextProxy, exercise simultaneous public profile activation, verify reload after profile/config changes, or cover multi-view reset/import/disposal, so it does not detect the failures above.
taltas
left a comment
taltas
left a comment
There was a problem hiding this comment.
Request changes: 5 blocking state-isolation issues remain.
Evidence: focused changed suites passed (81 extension-host tests and 18 webview tests); both typechecks and the 1,458-test webview suite passed. The full extension suite reached 6,816 passing assertions but exited on six teardown errors from the new parallel-mode suite.
Superseded by concise inline review comments.
github-actions
Bot
added
awaiting-author
…ared data snapshot
…er settings import
|
Thanks for the thorough review, @taltas! Here are my responses to your findings: 1. Cross-view configuration merging (credentials) 2. Persistence doesn't survive ContextProxy reconstruction 3. Mode-specific profile selection is globally shared 4. Globally shared mode data is frozen into local buffer 5. Global reset/import leaves other live views on stale state I've also updated the PR description to reflect these design decisions and known limitations. Let me know if you're happy with this approach! |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/webview/ClineProvider.ts (1)
1846-1848: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDerive the replacement profile from this view, not shared state.
Line 1847 applies
profileToActivatelocally, but that value originates from sharedcontextProxy.getValues().currentApiConfigName. Deleting an unrelated profile can therefore switch this tab to another tab’s profile. Initialize it from(await this.getState()).currentApiConfigName.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/webview/ClineProvider.ts` around lines 1846 - 1848, Update the profile activation flow surrounding _updateViewLocalStateFromMutation to initialize profileToActivate from this view’s state via await this.getState().currentApiConfigName, rather than shared contextProxy values. Preserve the existing local-state update while ensuring unrelated profile deletion cannot switch this tab to another tab’s profile.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/core/webview/ClineProvider.ts`:
- Around line 1846-1848: Update the profile activation flow surrounding
_updateViewLocalStateFromMutation to initialize profileToActivate from this
view’s state via await this.getState().currentApiConfigName, rather than shared
contextProxy values. Preserve the existing local-state update while ensuring
unrelated profile deletion cannot switch this tab to another tab’s profile.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d3a6a5da-37b8-4b31-a0ca-9933ebe00134
📒 Files selected for processing (8)
src/core/config/__tests__/importExport.spec.tssrc/core/config/importExport.tssrc/core/webview/ClineProvider.tssrc/core/webview/__tests__/ClineProvider.parallelMode.spec.tssrc/core/webview/__tests__/webviewMessageHandler.spec.tssrc/core/webview/webviewMessageHandler.tswebview-ui/src/App.tsxwebview-ui/src/context/ExtensionStateContext.tsx
💤 Files with no reviewable changes (1)
- webview-ui/src/App.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- webview-ui/src/context/ExtensionStateContext.tsx
github-actions
Bot
added
awaiting-review
3 participants
Summary
Fixes multi-tab mode/model settings being overwritten by global state from other tabs. Each ClineProvider instance (sidebar, editor tab) now maintains its own isolated
mode,apiConfiguration, and profile selection via a per-view local state buffer with view-specific persistence keys.Problem
In multi-tab mode, when users switch between Roo Code tabs with different mode/API configuration selections, the active tabs settings get overwritten by the global state from another tab. This happens because all tabs share the same ContextProxy singleton — any
handleModeSwitch()oractivateProviderProfile()call in one tab updates shared global state that other tabs then read.Solution
Implemented view-local state isolation using a per-provider-instance buffer with view-specific persistence keys:
Architecture
nextViewIdstatic counter for stable, monotonically increasing view IDs ({renderContext}-{counter})viewLocalState: Partial<ExtensionState>buffer per provider instance__view_state_{viewId}_{key}so recreated views restore their own values instead of the last writer shared stategetState()to mergeviewLocalStateon top of global state with proper precedenceKey Changes in
src/core/webview/ClineProvider.ts:loadViewState()__view_state_{id}_mode), falls back to shared keys for backward compatibility. Only snapshots per-view fields (mode,currentApiConfigName,apiConfiguration) — project-level settings likecustomModePromptsandmodeApiConfigsare read fresh from ContextProxy each timesaveViewState(key, value)_updateViewLocalStateFromMutation(values)setValues(),setValue(), profile upsert/activation/deletion, orresetState()setupGlobalStateListener()handleModeSwitch()_updateViewLocalStateFromMutation+ reads currentApiConfigName fromgetState()(not direct global read) beforeupdateGlobalState()activateProviderProfile()/deleteProviderProfile()_updateViewLocalStateFromMutationfor consistent synccreateTaskWithHistoryItem()setValue()/setValues()_updateViewLocalStateFromMutationresetState()_clearViewLocalState()+broadcastResetToAllInstances()after resetting ContextProxybroadcastResetToAllInstances()View-Specific Persistence Keys:
Merge Precedence in
getState():Design Decisions (Per-View vs Project-Level Shared)
modecurrentApiConfigName/apiConfigurationcustomModePromptsmodeApiConfigs(mode→profile mapping)customInstructions,allowedCommands,mcpServersAdditional Changes (beyond ClineProvider)
src/extension/api.tsAPI.setConfiguration()now callssidebarProvider.setValues(values)instead of directly callingcontextProxy.setValues(), ensuring_updateViewLocalStateFromMutationis triggeredwebview-ui/src/utils/vscode.tsviewStateIdto webview launch handshake — each tab gets a unique ID stored in ContextProxy for cross-session persistencewebview-ui/src/App.tsx/ExtensionStateContext.tsxviewStateIdfrom extension host contextsrc/core/config/importExport.tsbroadcastResetToAllInstances()(wrapped in try-catch) to clear stale view-local state across parallel tabsTesting
22+ new unit tests across multiple test files:
ClineProvider.parallelMode.spec.ts— viewId uniqueness, mode/apiConfig isolation, saveViewState/loadViewState with undefined clearing, getState merge precedence (local overrides global), GlobalState listener handling, handleModeSwitch/activateProviderProfile integration, multi-instance isolation (3+ concurrent providers)api-set-configuration.spec.ts— verifiesAPI.setConfiguration()triggers_updateViewLocalStateFromMutationand propagates to view-local stateRegression: All 2179 existing core tests pass ✅
Known Limitations
viewLocalStatepersists across tab switches within the same extension host session, but falls back to shared global state on restart. View-specific keys (__view_state_*) are not inGLOBAL_STATE_KEYS, so they are not loaded duringContextProxy.initialize().GLOBAL_STATE_KEYSor usevscode.workspaceStateresetState()and settings import callbroadcastResetToAllInstances()to clear view-local state across all active tabs. Wrapped in try-catch so import resilience isn't broken by broadcast failures. Other views may briefly see stale state during the async broadcast window, or if a provider instance lacks the method (optional interface).modeApiConfigs) is project-level shared. If two tabs switch to the same mode, they follow the same global default (last writer wins). Per-tab mode→profile isolation is not yet implemented.modeApiConfigsContextProxyspread. This is expected behavior (shared memory space).Related Issue
Closes #908
Summary by CodeRabbit
New Features
viewStateIdfor better view state correlation.Bug Fixes
Tests
viewStateIdpropagation, state merging precedence, and reset/broadcast flows.